Skip to content

Conversation

amesgen
Copy link
Member

@amesgen amesgen commented Sep 2, 2025

@amesgen amesgen added the Peras label Sep 2, 2025
@amesgen amesgen changed the base branch from main to peras/object-diffusion September 2, 2025 10:05
Base automatically changed from peras/object-diffusion to peras-staging September 3, 2025 07:52
@agustinmista agustinmista self-assigned this Sep 3, 2025
@agustinmista
Copy link
Contributor

agustinmista commented Sep 3, 2025

Hey @amesgen @tbagrel1

The PR still needs a lot of work, but I think it would be good to get some early feedback on 7212954. I pushed a bunch of changes adapting call sites to use ValidatedPerasCerts whenever needed, as implied by the two TODOs left in 3425d67:

  • Adapting PerasCertDB.addCert to take a ValidatedPerasCert, and
  • Adapting ChainDB.addPerasCertAsync to take a ValidatedPerasCert

Based on my current understanding, there is a tension between keeping either PerasCerts or ValidatedPerasCerts around, depending on what we want to enforce:

  1. Validating certs early on, and using those everywhere
  • Somewhat better assurance that we don't ever try to operate on potentially-invalid certs
  • Potentially useful in the cases where the extra evidence they carry allows us to avoid recomputing stuff (e.g. the block's boost)
  • Needs a "big" refactor to adapt all call sites, including the "generation" of the now needed validation evidence in some property tests
  1. Validating certs when needed, but using the original PerasCert everywhere
  • Might open the door for bugs caused by operating on invalid certs, not sure how likely this is, though
  • When useful, validated certs (or at least their useful evidence) might need to be passed around manually all the way down to the consumer
  • Likely a smaller refactor, as we can probably unwrap ValidatedPerasCerts in a couple of places and keep the original type signature

Do you think (1) is the right approach here? Or is (2) something more akin to what we would want at this stage in the dev cycle?

@agustinmista agustinmista force-pushed the peras/basic-cert-validation-api branch 2 times, most recently from c5beff6 to 7212954 Compare September 3, 2025 11:22
@amesgen
Copy link
Member Author

amesgen commented Sep 3, 2025

I think (1) is indeed the right approach, the benefits seem compelling even at this stage.

(In the standup, we just briefly talked about some of the implications this has once we store certificates on disk, such as whether we also want to store the weight boost of a certificate on disk, or recompute it on startup. But this is not important for now.)

including the "generation" of the now needed validation evidence in some property tests

This is actually a desired outcome of this change, see the last paragraph in the issue description of #94. To keep this PR minimal, still always using boostPerCert sounds good as you currently do; enriching the generators could be follow-up work.

@agustinmista agustinmista force-pushed the peras/basic-cert-validation-api branch from 7212954 to 4f487d9 Compare September 3, 2025 13:41
Comment on lines 118 to 119
defaultPerasCfg :: PerasCfg blk
defaultPerasCfg = PerasCfg{perasCfgWeightBoost = boostPerCert}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another value that will have to be computed upon some external parameters, IIUC.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file also shouldn't change

@agustinmista agustinmista force-pushed the peras/basic-cert-validation-api branch 2 times, most recently from 3e437fe to 2cbdb3e Compare September 4, 2025 14:02
-- | Current round number
PerasRoundNo ->
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want to remove this again; I can't think of any block-specific criterion that would involve the current round number. So let's rather keep the API as simple as possible (it will get more complicated in the future when we add LedgerState-derived input).

@agustinmista agustinmista force-pushed the peras/basic-cert-validation-api branch 2 times, most recently from 062b507 to 750e881 Compare September 4, 2025 17:55
@agustinmista agustinmista force-pushed the peras/basic-cert-validation-api branch from 750e881 to 0857d5f Compare September 4, 2025 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add basic API for certificate validation
2 participants